refactor(inkless): return LogConfig from MetadataView#getTopicConfig #474
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the MetadataView#getTopicConfig method to return LogConfig instead of Properties, consolidating configuration merging logic into the MetadataView implementation. This simplifies callers by removing redundant LogConfig.fromProps() calls and eliminates the need to separately access default configurations.
Key changes:
- Interface change in
MetadataViewto returnLogConfiginstead ofProperties - Implementation in
InklessMetadataViewnow merges default and topic-specific configs internally - Simplified caller code in
AppendHandlerandRetentionEnforcer
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| storage/inkless/src/main/java/io/aiven/inkless/control_plane/MetadataView.java | Updated interface method signature to return LogConfig instead of Properties |
| core/src/main/scala/kafka/server/metadata/InklessMetadataView.scala | Implemented getTopicConfig to merge default configs with topic overrides using LogConfig.fromProps |
| storage/inkless/src/main/java/io/aiven/inkless/produce/AppendHandler.java | Simplified getLogConfigs method by removing LogConfig.fromProps call and refactored constructor to accept Function<String, LogConfig> |
| storage/inkless/src/main/java/io/aiven/inkless/delete/RetentionEnforcer.java | Replaced LogConfig.fromProps call with direct method reference to getTopicConfig |
| storage/inkless/src/test/java/io/aiven/inkless/produce/AppendHandlerTest.java | Simplified test setup by removing SharedState and related mock dependencies |
| storage/inkless/src/test/java/io/aiven/inkless/delete/RetentionEnforcerTest.java | Updated mocks to return LogConfig instead of Properties and added try-with-resources for proper cleanup |
| storage/inkless/src/test/java/io/aiven/inkless/merge/FileMergerIntegrationTest.java | Simplified mock setup to return LogConfig directly |
| storage/inkless/src/test/java/io/aiven/inkless/delete/FileCleanerIntegrationTest.java | Simplified mock setup to return LogConfig directly |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Change MetadataView#getTopicConfig to return LogConfig instead of Properties, consolidating the responsibility of merging default and topic-specific configurations into the MetadataView implementation. This simplifies callers by: - Removing redundant LogConfig.fromProps() calls in RetentionEnforcer and AppendHandler - Eliminating the need for callers to access default configs separately - Reducing test setup by removing mock configuration for default configs The InklessMetadataView now handles merging default configs with topic overrides internally, providing a fully-resolved LogConfig to consumers.
jeqo
force-pushed
the
jeqo/refactor-metadata-logconfig
branch
from
1dce5f5 to
6e7b51f
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Change MetadataView#getTopicConfig to return LogConfig instead of Properties, consolidating the responsibility of merging default and topic-specific configurations into the MetadataView implementation.
This simplifies callers by:
The InklessMetadataView now handles merging default configs with topic overrides internally, providing a fully-resolved LogConfig to consumers.